fix: only record a trace if needed by an audit #2117
Conversation
|
love it. can you add a quick test? |
|
done |
|
is it going to be weird that this only runs when creating a config with Also should we provide an escape hatch if you want to override this and record a trace (e.g. if you're doing --save-artifacts/assets to get a trace saved?) |
Seems fine to me, if you're explicitly including a trace without a reason, we shouldn't stand in your way.
Yeah I was slightly worried about this and other scenarios that you might sneakily consume a trace through unofficial channels or something. I'd prefer to leave that an outstanding issue until we add unification of CLI and config flags in |
|
aye. If you really want a trace, then including user-timings audit doesn't seem all that bad. |
I guess I meant weird the other way, that you could get a trace even if an audit didn't need one in your own config, but if you extend that config and whitelist everything suddenly it's gone |
|
I'd be ok with waiting for the great settings/flags rationalization, but maybe add a |
yeah good call on |
lighthouse-core/config/config.js
Outdated
|
|
||
| // disable the trace if no audit requires a trace | ||
| if (pass.recordTrace && !auditsNeedTrace) { | ||
| log.warn('config', `Trace not requested by an audit, dropping trace in ${pass.passName}`); |
There was a problem hiding this comment.
the only issue with this is that validatePasses hasn't run yet, so the pass might not have a passName (and we allow no name if there's only one unnamed pass, at which point it should be DEFAULT_PASS...ugh)
Maybe 'Trace not requested by an audit, dropping trace in pass '${pass.passName}'' so at least they get a pass 'undefined'? :)
(we should figure out ordering on that at some point, too. Makes sense to validate at the end, so the actual config that's going to be run is sure to be valid, but input is also supposed to be valid so...validate twice?)
| it('filtering filters out traces when not needed', () => { | ||
| const warnings = []; | ||
| const saveWarning = evt => warnings.push(evt); | ||
| log.events.addListener('warning', saveWarning); |
There was a problem hiding this comment.
remove listener when finished?
fixes #2088
needs coordination with #2062 and #2104